airbyte/mcp/cloud_ops.py (3)

925-933: Consider restructuring error messages to decouple dynamic values.

Based on learnings, error messages in PyAirbyte follow a structlog-inspired design where dynamic values (especially potential PII like organization names) are provided via the context parameter rather than embedded in the message string. This helps avoid including PII in telemetry.

Would you consider restructuring these errors? For example:

     if not matching_orgs:
         raise AirbyteMissingResourceError(
             resource_type="organization",
             context={
                 "organization_name": organization_name,
-                "message": f"No organization found with exact name '{organization_name}' "
-                "for the current user.",
             },
+            message="No organization found with exact name for the current user.",
         )
 
     if len(matching_orgs) > 1:
         raise PyAirbyteInputError(
-            message=f"Multiple organizations found with name '{organization_name}'. "
-            "Please use 'organization_id' instead to specify the exact organization."
+            message="Multiple organizations found with the specified name. "
+            "Please use 'organization_id' instead to specify the exact organization.",
+            context={"organization_name": organization_name},
         )

Based on learnings, this pattern helps prevent PII leakage in telemetry. Wdyt?

Also applies to: 935-939

---

1005-1012: Consider whether empty string defaults are appropriate here.

Using .get() with empty string defaults ("") means that if the API response is missing workspaceId, name, or organizationId fields, we'll create result objects with empty strings rather than failing fast or using None. This could mask data quality issues.

Would it be better to either:

1. Let it fail if required fields are missing (don't use defaults)
2. Use explicit validation to ensure required fields exist
3. Use None as the default if these fields can legitimately be absent

Wdyt? If these fields are always expected from the API, option 1 might be clearest.

---

1058-1090: Consider reusing _resolve_organization_id to avoid duplication.

This function duplicates the organization lookup logic from _resolve_organization_id (lines 878-942). The only difference is that this function needs to return the full organization object, while _resolve_organization_id returns just the ID.

What if you refactored to reuse the helper? For example:

@mcp_tool(...)
def describe_cloud_organization(
    *,
    organization_id: ...,
    organization_name: ...,
) -> CloudOrganizationResult:
    """Get details about a specific organization."""
    api_root = resolve_cloud_api_url()
    client_id = SecretString(resolve_cloud_client_id())
    client_secret = SecretString(resolve_cloud_client_secret())
    
    # Get all organizations
    orgs = api_util.list_organizations_for_user(
        api_root=api_root,
        client_id=client_id,
        client_secret=client_secret,
    )
    
    # Find the matching one
    if organization_id:
        matching_orgs = [org for org in orgs if org.organization_id == organization_id]
        search_key = "organization_id"
        search_value = organization_id
    elif organization_name:
        matching_orgs = [org for org in orgs if org.organization_name == organization_name]
        search_key = "organization_name"
        search_value = organization_name
    else:
        raise PyAirbyteInputError(
            message="Either 'organization_id' or 'organization_name' must be provided."
        )
    
    if not matching_orgs:
        raise AirbyteMissingResourceError(
            resource_type="organization",
            context={
                search_key: search_value,
            },
            message="No organization found for the current user.",
        )
    
    if len(matching_orgs) > 1:
        raise PyAirbyteInputError(
            message="Multiple organizations found. Please use 'organization_id' instead.",
            context={search_key: search_value},
        )
    
    org = matching_orgs[0]
    return CloudOrganizationResult(
        id=org.organization_id,
        name=org.organization_name,
        email=org.email,
    )

This also addresses the error message structure concerns by moving dynamic values to context. Wdyt?

airbyte/_util/api_util.py (1)

1646-1655: Consider adding validation for the API response structure.

The function assumes json_result.get("workspaces", []) will work, but if the API response structure changes or errors occur, this might mask issues. The function relies entirely on _make_config_api_request to raise errors.

Would it be worth adding a quick validation check? Something like:

        json_result = _make_config_api_request(
            path="/workspaces/list_by_organization_id",
            json=payload,
            api_root=api_root,
            client_id=client_id,
            client_secret=client_secret,
        )
        
        if "workspaces" not in json_result:
            raise AirbyteError(
                message="Unexpected API response structure: missing 'workspaces' field",
                context={
                    "organization_id": organization_id,
                    "response_keys": list(json_result.keys()),
                },
            )

        workspaces = json_result["workspaces"]

This would make debugging easier if the API changes. That said, it's a fairly defensive approach - the current implementation is reasonable too. Wdyt?

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: aaronsteers
Repo: airbytehq/PyAirbyte PR: 411
File: airbyte/cli.py:111-160
Timestamp: 2024-10-06T23:44:31.534Z
Learning: In PyAirbyte, error messages in functions like `_resolve_source_job` in `airbyte/cli.py` are designed to decouple the message text from dynamic values, following a structlog-inspired design. Dynamic values are provided via parameters like `input_value`. This approach helps avoid including PII in the message strings, which may be used in telemetry.

airbyte/mcp/cloud_ops.py (3)

917-921: Type mismatch issue may still be present.

Previous review comments flagged that resolve_cloud_client_id() and resolve_cloud_client_secret() return str rather than SecretString, but this function expects SecretString parameters. Based on the relevant code snippets, these functions should return SecretString, but if the pipeline is still failing with type errors, you may need to explicitly wrap the return values.

The same issue applies to the callers at lines 989-995 and 1052-1056.

Could you verify whether the pipeline failures are resolved? If not, you might need to wrap the credentials like:

     orgs = api_util.list_organizations_for_user(
         api_root=api_root,
-        client_id=client_id,
-        client_secret=client_secret,
+        client_id=SecretString(client_id) if isinstance(client_id, str) else client_id,
+        client_secret=SecretString(client_secret) if isinstance(client_secret, str) else client_secret,
     )

What do you think?

---

989-1004: Type mismatch propagates here as well.

Similar to the issue in _resolve_organization_id, these calls pass credentials that may need SecretString wrapping. This duplicates the concerns from the previous review.

The same wrapping approach would apply here. Could you confirm if the pipeline failures are resolved?

---

1052-1056: Type mismatch here as well.

Same issue as the previous calls: credentials may need SecretString wrapping per the previous review comments.

The same fix would apply here, wdyt?

airbyte/mcp/cloud_ops.py (2)

904-940: Consider structlog-inspired error message design for consistency.

Based on learnings from the codebase, PyAirbyte error messages typically decouple the message text from dynamic values to avoid including potential PII in telemetry. Organization names and IDs might be considered sensitive.

Would you consider refactoring these error messages to use the context parameter for dynamic values? For example:

raise PyAirbyteInputError(
    message="Either 'organization_id' or 'organization_name' must be provided.",
    context={
        "organization_id": organization_id,
        "organization_name": organization_name,
    },
)

This pattern is used elsewhere in the codebase (see lines 1064-1069 and 1076-1081 in describe_cloud_organization), wdyt?

Based on learnings, PyAirbyte prefers decoupling message text from dynamic values to avoid PII in telemetry strings.

---

1059-1091: Consider reducing code duplication with _resolve_organization_id.

The logic here for resolving an organization by ID or name is very similar to what _resolve_organization_id does (lines 879-943). The main difference is that this function returns the full organization object while _resolve_organization_id returns just the ID.

Would you consider refactoring to call _resolve_organization_id first to get the ID, then look up the full org from the list? This would reduce duplication and make the validation logic consistent. For example:

# Get all organizations for the user
orgs = api_util.list_organizations_for_user(
    api_root=api_root,
    client_id=client_id,
    client_secret=client_secret,
)

# Resolve to ID using shared helper
resolved_org_id = _resolve_organization_id(
    organization_id=organization_id,
    organization_name=organization_name,
    api_root=api_root,
    client_id=client_id,
    client_secret=client_secret,
)

# Find the full org object
org = next((o for o in orgs if o.organization_id == resolved_org_id), None)
if not org:
    raise AirbyteMissingResourceError(...)

This is just a suggested refactor to improve maintainability, wdyt?

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: aaronsteers
Repo: airbytehq/PyAirbyte PR: 411
File: airbyte/cli.py:111-160
Timestamp: 2024-10-06T23:44:31.534Z
Learning: In PyAirbyte, error messages in functions like `_resolve_source_job` in `airbyte/cli.py` are designed to decouple the message text from dynamic values, following a structlog-inspired design. Dynamic values are provided via parameters like `input_value`. This approach helps avoid including PII in the message strings, which may be used in telemetry.

airbyte/_util/api_util.py (1)

1607-1669: Workspace pagination logic looks good; consider reusing the bearer token for many-page orgs?

The paging loop, name_contains wiring, and max_items_limit behavior all look solid and address the earlier “no while True” feedback nicely. One minor thing: each call to _make_config_api_request re-does the bearer-token exchange, which is fine for typical “few-page” orgs but could add noticeable overhead if an admin ever has hundreds of workspaces.

Would you consider a follow-up that lets _make_config_api_request optionally accept a pre-fetched bearer token (so a single token can be reused across pages in this loop), or do you prefer to keep the simpler API and only revisit if this becomes a hot path, wdyt?

airbyte/mcp/cloud_ops.py (4)

126-146: New CloudOrganizationResult / CloudWorkspaceResult models look clean; should org email allow None?

The shapes for CloudOrganizationResult and CloudWorkspaceResult line up nicely with the underlying API fields and give MCP callers a simple, stable contract. One small question: depending on the Airbyte API schema, OrganizationResponse.email might be nullable in some edge cases; if that’s possible, would it be safer to declare email: str | None on CloudOrganizationResult (or default to "") to avoid surprises when pydantic validates a None, wdyt?

---

879-953: Org resolution logic is solid; consider aligning error messages with the “no PII in message text” convention.

The overall flow here is great: you enforce “either id or name, but not both”, resolve against the current user’s orgs via list_organizations_for_user, and handle both not-found and ambiguous-name cases with the right exception types.

Given the prior guidance about keeping error messages static and putting dynamic values into context fields, would you consider:

• For the “multiple organizations found” branch, using a static message (e.g. "Multiple organizations found with this name. Please provide 'organization_id' instead.") and passing organization_name in context or input_value instead of interpolating it directly into the message string?
• Similarly, for the not-found-by-name/ID cases, keeping any user- or org-specific identifiers out of the human-readable text and relying on context keys like "organization_id" / "organization_name"?

That would keep this consistent with the structlog-style pattern you’ve used elsewhere and avoid PII in top-level messages, wdyt?

---

985-1047: list_cloud_workspaces correctly enforces org scoping; would adding workspace URL/slug be useful to callers?

This tool seems to hit all the design goals:

• It requires explicit organization context (ID or exact, case-sensitive name) and delegates validation to _resolve_organization_id.
• It uses the new list_workspaces_in_organization helper so the Config API does the heavy lifting, including server-side name_contains filtering.
• It returns a minimal, well-typed CloudWorkspaceResult list that’s easy for agents to consume.

One question: since the underlying payload includes fields like slug and (presumably) a way to construct the workspace web URL, would it be worth exposing either slug and/or a url field on CloudWorkspaceResult to make it easier for tools/agents to surface links without an extra lookup, or are you intentionally keeping this result schema minimal for now, wdyt?

---

1056-1096: describe_cloud_organization nicely reuses the resolver; just double-check email is always non-null.

Using _resolve_organization here keeps the “exact name vs ID” behavior, ambiguity checks, and not-found handling perfectly in sync with list_cloud_workspaces, which is great for predictability. Returning a compact CloudOrganizationResult with id, name, and email seems exactly in line with the “name ↔ ID lookup” use case.

Similar to the earlier model comment: if OrganizationResponse.email can ever be None, should this tool either (a) type email as str | None or (b) normalize None to "" before constructing CloudOrganizationResult, to avoid subtle validation/runtime issues, wdyt?

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: aaronsteers
Repo: airbytehq/PyAirbyte PR: 411
File: airbyte/cli.py:111-160
Timestamp: 2024-10-06T23:44:31.534Z
Learning: In PyAirbyte, error messages in functions like `_resolve_source_job` in `airbyte/cli.py` are designed to decouple the message text from dynamic values, following a structlog-inspired design. Dynamic values are provided via parameters like `input_value`. This approach helps avoid including PII in the message strings, which may be used in telemetry.